-
Notifications
You must be signed in to change notification settings - Fork 177
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HIP: Add Exclude File Option to Helm Lint Command #353
base: main
Are you sure you want to change the base?
Conversation
3a6938e
to
035b33d
Compare
Signed-off-by: Danilo Patrucco <danilo.patrucco@gmail.com> Signed-off-by: danilo-patrucco <danilo.patrucco@sev1tech.com> Signed-off-by: Danilo Patrucco <danilo.patrucco@gmail.com> Signed-off-by: Danilo Patrucco <danilo.patrucco@sev1tech.com>
035b33d
to
1385741
Compare
Add Daniel Pritchett due to his participation in the feature development Signed-off-by: danilo patrucco <danilo.patrucco@gmail.com>
See HIP-0019 proposal at helm/community: helm/community#353 Co-authored-by: Danilo Patrucco <danilo.patrucco@gmail.com> Signed-off-by: Daniel J. Pritchett <dpritchett@radiusmethod.com>
See HIP-0019 proposal at helm/community: helm/community#353 Co-authored-by: Danilo Patrucco <danilo.patrucco@gmail.com> Signed-off-by: Daniel J. Pritchett <dpritchett@radiusmethod.com>
See HIP-0019 proposal at helm/community: helm/community#353 Co-Authored-By: Danilo Patrucco <danilo.patrucco@gmail.com> Signed-off-by: Daniel J. Pritchett <dpritchett@radiusmethod.com>
See HIP-0019 proposal at helm/community: helm/community#353 Co-authored-by: Danilo Patrucco <danilo.patrucco@gmail.com> Signed-off-by: Daniel J. Pritchett <dpritchett@radiusmethod.com>
- adds ignorer usage to cmd/helm/lint.go - adds ignorer usage to pkg/action/lint.go and its lint_test.go See HIP-0019 proposal at helm/community: helm/community#353 Co-authored-by: Danilo Patrucco <danilo.patrucco@gmail.com> Signed-off-by: Daniel J. Pritchett <dpritchett@radiusmethod.com>
hips/hip-0019.md
Outdated
The `.helmlintignore` file will support simple patterns to match files and directories for easy exclusion management. The `--lint-ignore-file` flag enhances this by allowing centralized management of lint exclusions in complex projects. | ||
## Specification | ||
### `.helmlintignore` File Format | ||
The `.helmlintignore` file allows chart developers to specify filenames or glob patterns to exclude from linting. The format is straightforward, with one pattern per line, similar to `.gitignore` files. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a yaml format that can encode different types of rules/options for linting would be much more preferable than a custom / one-file-per-line format (that is limited to just that).
I also think that all/nothing for lint ignoring (per-file) is too coarse for the general case. Especially since many lint rules pertain (in Helm) to e.g. Chart.yaml
(ref). Likely it normally would be better to exclude certain rules for Chart specific files (Chart.yaml
, values.yaml
(default values), etc), and paths for templates, etc. See https://github.com/helm/helm/tree/main/pkg/lint/rules for current Helm linting rules.
Also take e.g. https://yamllint.readthedocs.io/en/stable/configuration.html#configuration or https://ansible.readthedocs.io/projects/lint/configuring/#ansible-lint-configuration as inspirations/examples for what might go into a lint configuration (of course not every there will apply to Helm).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GeorgeJ
I have a question about this comment:
I also think that all/nothing for lint ignoring (per-file) is too course for the general case. Especially since many lint rules pertain (in Helm) to e.g. Chart.yaml (ref). Likely it normally would be better to exclude certain rules for Chart specific files (Chart.yaml, values.yaml (default values), etc), and paths for templates, etc. See https://github.com/helm/helm/tree/main/pkg/lint/rules for current Helm linting rules.
In response this discussion about the broad approach to lint ignoring, especially for key Helm config files like Chart.yaml, I want to discuss the current errors handling from the PR that we submitted. Right now we remove errors rather than skipping them altogether
Here’s the gist:
Error removal: We let all errors get logged as usual but we remove them from the final error list that users see. This keeps the linting thorough but the user interface clean.
Debugging Option: If someone needs to dive deep or is troubleshooting, they can use the --debug flag to see everything, removed errors included.
Post-Processing Advantages: Handling the error list after it’s fully formed means we clean things up at the end. This makes managing exceptions way easier and ensures we’re not tossing useful feedback too early.
I see a few pluses here:
Flexible Error Reports: You get a neat, easy-to-digest error report by default but can switch to a detailed view when needed.
Better Troubleshooting: Keeping all the linting info till the end helps us understand what we're filtering out and why, making it easier to adjust if something's not quite right.
What you are suggesting would be more aligned with something like this :we would need to change the structure of the lintError variable to something like this:
type LintError struct {
// Err is the original linter error
Err error
// ChartPath is the string path to the chart that generated this linter error
ChartPath string
// TemplatePath is nullable because some errors are chart-level while others are template-specific
TemplatePath *string
}
In this way we would be able to better review and analyze the errors and even list them in the output, giving an idea of how many errors are ignored, but that would require a pretty important lift, because it would change multiple files/potentially structures/functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reply from @gjenkins8
Morning, I don't disagree with the above (the "post-filtering" approach). I took a look at the reworked HIP, it looks good to me! Thanks for the updates
If the idea of specific rule based exclusions is too much for the moment, I think fine to pivot and just support what you have described (the fileMask and errorMask "directives")
I would suggest to small updates the HIP with some scope details:
only file / error based exclusions (masks) considered
rule-based exclusions (& configuration) considered difficult to implement, and skipped for the moment
My major/main goal was to make sure e.g. like yaml lint that the lint configuration can be extended in the future. I think it can with the current format (but I will put some comments on the format on the HIP)
Signed-off-by: Danilo Patrucco <danilo.patrucco@gmail.com>
#### Example | ||
``` | ||
# .helmlintconfig.yaml file example | ||
lintIgnore: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps a format more like (bikeshedding hard here):
# .helmlintconfig.yaml file example
pathIgnore:
- "migrations/templates/job.yaml"
- "gitlab/templates/shared-secrets/self-signed-cert-job.yml"
- "gitlab/templates/*.yaml"
errorIgnore:
- "chart metadata is missing these dependencies*"
- "<include 'fullname' .>"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I modified this section a little, but kept it similar to our suggestion because an error (the same error specifically) can occur in multiple charts / sub-charts, and connecting it to a file path would be beneficial (sorry for the bikeshedding here)
- Developing an effective and user-friendly pattern syntax for the `.helmlintconfig.yaml` file. | ||
- Considering the potential for in-file annotations to temporarily exclude specific chart sections from linting. | ||
|
||
## Rejected Ideas |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Providing rule specific excludes and configurations, and composing configuration from "default" configuration e.g. similar to https://yamllint.readthedocs.io/en/stable/configuration.html#extending-the-default-configuration
Helm today doesn't "name" its lint rules, and changing the code/structure to support the above can be extended later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, let me know if it's ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, sorry, I mean we should add these to the "rejected ideas":
- Providing rule specific excludes (Helm today doesn't "name" its lint rules)
- composing configuration from "default" configuration (e.g. similar to https://yamllint.readthedocs.io/en/stable/configuration.html#extending-the-default-configuration)
hips/hip-0019.md
Outdated
|
||
## Specification | ||
### `.helmlintconfig.yaml` File Format | ||
The `.helmlintconfig.yaml` file enables chart developers to specify filenames or glob patterns to exclude from linting. The format is YAML, with patterns specified under a `lintIgnore` key, making future extensions for additional configurations feasible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
glob patterns for both file paths and error strings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done !
Signed-off-by: Danilo Patrucco <danilo.patrucco@gmail.com>
hips/hip-0019.md
Outdated
--- | ||
|
||
## Abstract | ||
This proposal introduces enhancements to the `helm lint` command, allowing the exclusion of specific files or patterns through a `.helmlintconfig.yaml` file, analogous to `.gitignore` or `.helmignore`. Additionally, a `--lint-config-file` flag will be added to specify alternative locations for the `.helmlintconfig.yaml` file. This feature is particularly useful in projects with multiple sub-charts. An environment variable `HELM_LINT_CONFIG_FILE` will also be introduced to mirror the functionality of the flag. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we reword analogous to .gitignore or .helmignore
now that we're targeting a general-purpose config file format?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
hips/hip-0019.md
Outdated
This proposal introduces enhancements to the `helm lint` command, allowing the exclusion of specific files or patterns through a `.helmlintconfig.yaml` file, analogous to `.gitignore` or `.helmignore`. Additionally, a `--lint-config-file` flag will be added to specify alternative locations for the `.helmlintconfig.yaml` file. This feature is particularly useful in projects with multiple sub-charts. An environment variable `HELM_LINT_CONFIG_FILE` will also be introduced to mirror the functionality of the flag. | ||
|
||
## Motivation | ||
In large Helm charts, certain files or configurations may not conform to standard linting rules but are accepted by the maintainers. The current `helm lint` process evaluates all files within a chart, often leading to irrelevant warnings or errors. An option to exclude specific files and the ability to specify config file locations would streamline the linting process. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential phrasing: this can put helm users in situations where imported subcharts are flagged with low-priority lint errors that they'd prefer to suppress
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
hips/hip-0019.md
Outdated
``` | ||
|
||
### Command Behavior | ||
When `helm lint` is executed, it checks for the presence of a `.helmlintconfig.yaml` file in the chart directory or at a location specified by the `--lint-config-file` flag. The command then parses this file to exclude any files or directories matching the patterns specified, including specific error lines. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or wherever the env var says to look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Documentation for the `.helmlintconfig.yaml` file and the `--lint-config-file` flag will be integrated into the official Helm documentation under the `helm lint` section. Detailed examples and common patterns will be provided to assist new users. | ||
|
||
## Reference Implementation | ||
PR #[13257](https://github.com/helm/helm/pull/13257) created in the Helm/helm repository in gitlab |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
n.b. this PR will need some significant changes to catch up with the current design as described here in HIP-0019
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this i will leave untouched if it's ok with you, since we put the PR in draft
PR #[13257](https://github.com/helm/helm/pull/13257) created in the Helm/helm repository in gitlab | ||
|
||
## Open Issues | ||
- Developing an Effective and User-Friendly Pattern Syntax: We are considering the design of a flexible and intuitive pattern syntax for the `.helmlintconfig.yaml` file. This syntax would ideally support rule-specific configurations and exclusions, allowing users to finely control lint behaviors on a per-rule basis. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this piece is not self-evident to me, can we add a hypothetical such as "errorCode: H019
would allow for across-the board suppression of errors by error code. We are deferring this for now because it will require a refactor of all 50+ existing helm lint cases."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
hips/hip-0019.md
Outdated
## Open Issues | ||
- Developing an Effective and User-Friendly Pattern Syntax: We are considering the design of a flexible and intuitive pattern syntax for the `.helmlintconfig.yaml` file. This syntax would ideally support rule-specific configurations and exclusions, allowing users to finely control lint behaviors on a per-rule basis. | ||
- In-File Annotations for Excluding Chart Sections: Exploring the possibility of in-file annotations that would let developers temporarily exclude specific sections of a chart from linting. This feature would provide granular control directly within Helm chart files. | ||
- Rule Naming and Configurations: Currently, Helm does not support named lint rules. Introducing named rules is a foundational step that would enable us to implement more sophisticated configuration features, such as rule-specific exclusions and settings. This would also allow users to extend or override a "default" configuration set, similar to practices seen in tools like [yamllint](https://yamllint.readthedocs.io/en/stable/configuration.html#extending-the-default-configuration). Naming rules would facilitate clearer and more maintainable configurations, laying the groundwork for future enhancements. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like bullet points 1 and 3 here might be overlapping, maybe we could consolidate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
hips/hip-0019.md
Outdated
- Developing an Effective and User-Friendly Pattern Syntax: We are considering the design of a flexible and intuitive pattern syntax for the `.helmlintconfig.yaml` file. This syntax would ideally support rule-specific configurations and exclusions, allowing users to finely control lint behaviors on a per-rule basis. | ||
- In-File Annotations for Excluding Chart Sections: Exploring the possibility of in-file annotations that would let developers temporarily exclude specific sections of a chart from linting. This feature would provide granular control directly within Helm chart files. | ||
- Rule Naming and Configurations: Currently, Helm does not support named lint rules. Introducing named rules is a foundational step that would enable us to implement more sophisticated configuration features, such as rule-specific exclusions and settings. This would also allow users to extend or override a "default" configuration set, similar to practices seen in tools like [yamllint](https://yamllint.readthedocs.io/en/stable/configuration.html#extending-the-default-configuration). Naming rules would facilitate clearer and more maintainable configurations, laying the groundwork for future enhancements. | ||
- Future Enhancements and Extensibility: The proposed changes to the Helm linting system will likely require significant modifications to the existing codebase. As such, these enhancements could be implemented in phases, starting with foundational features like rule naming and extending to more complex configurations. This phased approach will help ensure stability and usability while expanding the linting capabilities of Helm. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The proposed changes to the Helm linting system will likely require significant modifications
The basic design in this HIP won't dramatically alter the helm codebase, but pretty much all of the stretch goals here in "open issues" would require some broader helm refactors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
hips/hip-0019.md
Outdated
--- | ||
|
||
## Abstract | ||
This proposal introduces enhancements to the `helm lint` command, allowing the exclusion of specific files or patterns through a `.helmlintconfig.yaml` file, analogous to `.gitignore` or `.helmignore`. Additionally, a `--lint-config-file` flag will be added to specify alternative locations for the `.helmlintconfig.yaml` file. This feature is particularly useful in projects with multiple sub-charts. An environment variable `HELM_LINT_CONFIG_FILE` will also be introduced to mirror the functionality of the flag. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This proposal introduces enhancements to the `helm lint` command, allowing the exclusion of specific files or patterns through a `.helmlintconfig.yaml` file, analogous to `.gitignore` or `.helmignore`. Additionally, a `--lint-config-file` flag will be added to specify alternative locations for the `.helmlintconfig.yaml` file. This feature is particularly useful in projects with multiple sub-charts. An environment variable `HELM_LINT_CONFIG_FILE` will also be introduced to mirror the functionality of the flag. | |
This proposal introduces enhancements to the `helm lint` command, allowing the exclusion of specific files or patterns through a `.helmlintignore` file, analogous to `.gitignore` or `.helmignore`. Additionally, a `--lint-config-file` flag will be added to specify alternative locations for the `.helmlintignore` file. This feature is particularly useful in projects with multiple sub-charts. An environment variable `HELM_LINT_IGNORE_FILE` will also be introduced to mirror the functionality of the flag. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Terry, Following our discussion with George, we've changed the .helmlintignore file to .helmlintconfig.yaml. The reason for this change is that the new config file will be in YAML format, and it will not only allow us to ignore specific linting errors but in the future we will also add more options/rules for linting. This is similar to the hadolint.yaml file that supports ignoring errors and setting up specific linting rules and options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, We did put the PR for helm/helm back in Draft, since we are waiting to have a finalized version of the HIP to continue the development work on the helm repo. Please let me know If you have any additional reservation, and thank you for the review
Signed-off-by: Danilo Patrucco <danilo.patrucco@gmail.com>
Signed-off-by: Danilo Patrucco <danilo.patrucco@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTMe
Final touchup would be adding the rejected ideas:
https://github.com/helm/community/pull/353/files#r1788505454
Perhaps some other eyes on the file format. Since this is the main user facing aspect, and important to get the UX right.
Signed-off-by: Danilo Patrucco <danilo.patrucco@gmail.com>
65ba1ba
to
4e07227
Compare
@gjenkins8 fixed the changes |
@mattfarina Hi Matt, would you be able to review this PR in addition to George and give us also your opinion on the file format ? |
Added HIP 0019 for the development of the lint ignore feature in helm binaries
refer to issue in helm repository #13133